Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed .csslintrc file support to use CSSLint specification #6

Closed
wants to merge 7 commits into from
Closed

Changed .csslintrc file support to use CSSLint specification #6

wants to merge 7 commits into from

Conversation

stevenbenner
Copy link

This pull request is related to the discussion in issue #3.

What this patch adds

  • Support for .csslintrc files in their native format.
  • The ability to disable linting rules via the --ignore directive.
  • The ability to exclude specific files from linting via the --exclude-list directive.
  • The ability to set the warning level of rules via the --errors and --warnings directives. (But they don't really affect this grunt task because even warnings will cause the task to fail and all rules are set to warnings by default)

What this patch does not add

The official .csslintrc file spec allows for any command line option. I left out support for the ones that I didn't think there was any reason to support. These include:

  • --help - Obviously pointless in a grunt task.
  • --format - Implementing this really didn't make sense to me because it doesn't really lend itself to working with the way formatters work now.
  • --list-rules - Doesn't belong in a grunt task.
  • --quiet - Would anyone really like for this to affect a grunt task?
  • --version - Another one that obviously doesn't belong in a grunt task.
  • It is also possible to add files to be linted via the .csslintrc file. I was debating whether I should implement that or not, but I decided against it because grunt users must explicitly list the files/directories that they want the task to run on.

There is certainly room for discussion about supporting the omitted features. Let me know your thoughts.

Behavior changed by this patch

  • This pull request drops support for JSON .csslintrc files completely. Anyone who has built a JSON .csslintrc file will find their csslint task broken when they upgrade.

Code notes

  • I added the .csslintrc parsing code in a file outside of the core task js. It felt more like a small library when I was writing it.
  • It would be much better to use CSSLint to process config files, but CSSLint does not expose any of the CLI code where the .csshintrc related code resides. Perhaps we should consider requesting (or submitting a pull request) that CSSLint exposes the argument processing code somewhere.
  • The rcparser exposes several convenience methods. These methods centralize the maintenance points that need to be touched if CSSLint ever changes their argument names or spec. However these might just be wasted code depending on how you look at it.
  • I used the node path library to determine the basePath for the .csslintrc file and to do the file exclusion comparisons because that just feels right, but I can't find anywhere else in grunt that behaves this way. This might be an undesirable practice in grunt plugins for some reason.

@jzaefferer
Copy link
Member

I veto against landing this until we at least petition the CSSLint folks to change the format of their .csslintrc file to be consistent with what jshint does, and what we implemented now. That format is much more sane and works nicely when moving from options to rc file or back: jquery/jquery-ui@39a2f46

@tkellen
Copy link
Member

tkellen commented Mar 15, 2013

I'll hold off on this until you've done so. Would you please /cc me on the issue you make over @ CSSLint?

@asciidisco
Copy link
Contributor

@jzaefferer Have you pinged @stubbornella or @nzakas already?
I think the jshintrc style is definitely an improvement in readability, it might be overhead, but we could do a type checking of the csslintrc contents and pick the right parser.

Another thing is, if any of the editor plugin developers wrote its own implementation of a csslintrc parser instead of using the CSSLint internal stuff, they have to change it also. I'm totally behind the idea, but I think it will be a rocky road.

@stevenbenner
Nicely done PR (in fact much better than any PR I've ever done), will scan the code later this evening.

@stevenbenner
Copy link
Author

@jzaefferer @tkellen Any news on this?

If you guys are waiting for a resolution on getting CSSLint to support s JSON configuration file then I think that at the very least the existing JSON support implemented in this plugin should be removed because it is very unlikely that whatever spec CSSLint settles on will be the same as we find in the current version of this plugin.

When you search GitHub for .csslintrc you will find that there is a growing number of repositories that have implemented .csslintrc files with a spec that exists nowhere in the universe except for grunt-contrib-csslint.

Like it or not, it is important to understand that official grunt plugins (and jQuery UI which is using this plugin) have some serious clout in the JavaScript world and people assume that the way it's done in these repositories is the right way to do things.

@jzaefferer
Copy link
Member

@stevenbenner no news, no, except that Nicholas declared not being able to deal with CSSLint at all. It looks like the last commit from Nicole was in December and there were no commits for two months now. So expecting anything to change within CSSLint is pretty unrealistic at this point.

The SublimerLinter devs would also favor a JSON format that doesn't require a custom parser.

As for this comment:

whatever spec CSSLint settles on will be the same as we find in the current version of this plugin

You may be right, but I'd like to at least point out that this format is actually used by CSSLint itself. They don't support it for the .csslintrc file, but its exactly what the programmatic API uses. Which explains why there's very little code in the plugin to convert the format in use to what CSSLint expects.

Anyway, since CSSLint isn't moving anywhere, merging this PR is probably the sanest thing to do.

@stubbornella
Copy link

So, help me catch up. It seems that several people would like to change the way configuration is done in CSS Lint to match how it is done in JSHint, am I right? But doing so would mean we needed to parse json?

So, I can see the value in the former, how big a pain point is the latter?

@jzaefferer
Copy link
Member

Parsing and outputting JSON is trivial, since we can just use JSON.parse and JSON.stringify. The more interesting question then is how to actually structure the data. The format CSSLint uses internally has a key per rule and a value to set each rule to ignore, warn or error, like this:

{
  "qualified-headings": true,
  "unique-headings": true,
  "known-properties": false
}

The CLI arguments have lists of rules for each of these states, which this PR uses:

--ignore=box-model,ids,important
--warn=unique-headings

Switching to JSON could mean using that first format, or converting that second format to a JSON structure, for example like this:

{
  "ignore": ["box-model", "ids", "important"],
  "warn": ["unique-headings"]
}

The first format, which this plugin currently (incorrectly) uses matches the format used by JSHint as well as what CSSLint uses internally.

The last format is also what Scott suggested in his comment here. He even has a (trivial) patch to add JSON support in addition of the current format.

@robwierzbowski
Copy link

The rule key/value pair is a common pattern, and more human readable and versionable. +1 from me.

@sindresorhus
Copy link
Member

ping

merge conflict needs to be resolved.

@stevenbenner
Copy link
Author

@sindresorhus Are you pinging me?

I see that support for JSON config files just got added to CSSLint, and will be available in the next release. That probably makes this pull request redundant. But if you would like to add support for the old fashion config files I suppose I could revisit this.

@sindresorhus
Copy link
Member

@stevenbenner yes, just wanted to clean up some dormant prs. we'll wait for the new csslint release.

@stevenbenner stevenbenner deleted the csslintrc-spec branch March 17, 2014 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants